-
Notifications
You must be signed in to change notification settings - Fork 902
GlobalOpenTelemetry.get()
should never returns obfuscated Noop OpenTelemetry
#7452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
…Telemetry Fix inconsistent behavior that first call will return Noop but later calls return obfuscated Noop. Signed-off-by: Yanming Zhou <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7452 +/- ##
============================================
+ Coverage 89.77% 89.82% +0.04%
- Complexity 6994 7000 +6
============================================
Files 798 798
Lines 21200 21199 -1
Branches 2058 2056 -2
============================================
+ Hits 19033 19041 +8
+ Misses 1503 1497 -6
+ Partials 664 661 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine. @jack-berg any reason not to make this change?
assertThat(GlobalOpenTelemetry.get()).isSameAs(OpenTelemetry.noop()); | ||
// ensure sequential calls of GlobalOpenTelemetry.get() return same object | ||
assertThat(GlobalOpenTelemetry.get()).isSameAs(OpenTelemetry.noop()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that storing the result of OpenTelemetry.noop()
as a local variable and using that for both comparisons would be a small improvement on this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, this is a little odd to me, because it requires knowledge of two inner layers -- the fact that OpenTelemetry.noop()
returns a static instance isn't really part of the contract, but this expects it to be...and it only is that way because the implementation of DefaultOpenTelemetry
has it that way, also not in the contract. 🤷🏻
It's probably fine, though, but I'm wondering what problem it solves.
@@ -88,7 +88,7 @@ public static OpenTelemetry get() { | |||
} | |||
} | |||
} | |||
return openTelemetry; | |||
return openTelemetry.delegate == OpenTelemetry.noop() ? OpenTelemetry.noop() : openTelemetry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should flip this such that GlobalOpenTelemetry.get()
always returns obfuscated opentelemetry, regardless of whether its noop or another instance. Something like:
public static OpenTelemetry get() {
OpenTelemetry openTelemetry = globalOpenTelemetry;
if (openTelemetry == null) {
synchronized (mutex) {
openTelemetry = globalOpenTelemetry;
if (openTelemetry == null) {
OpenTelemetry autoConfigured = maybeAutoConfigureAndSetGlobal();
if (autoConfigured != null) {
return autoConfigured;
}
set(OpenTelemetry.noop());
return Objects.requireNonNull(globalOpenTelemetry);
}
}
}
return openTelemetry;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should flip this such that
GlobalOpenTelemetry.get()
always returns obfuscated opentelemetry, regardless of whether its noop or another instance.
See my comment above, I need to check whether global OpenTelemetry
is registered.
I think it's fine to treat noop specially since it's exposed by public API OpenTelemetry.noop()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think comparison to OpenTelemetry.noop()
is the right way to do that.
@open-telemetry/java-approvers - I think we need to be consistent here and obfuscate either ALL or NONE of the responses to calling GlobalOpenTelemetry.get()
. And of course the risk with not obfuscating is that API users could cast to OpenTelemetrySdk
and call things like shutdown, flush.
We could choose to non obfuscate and have the autoconfigure module obfuscate before calling GlobalOpenTelemetry.set()
, but I think this still exposes too much of a foot gun. Too easy to accidentally set GlobalOpenTelemetry
to a non-obfuscated version and run into the problems above.
The core thing that @quaff is asking for has come up before: How can I check if GlobalOpenTelemetry
is set without causing the side affect of setting it?
If we want to support this, we should add something like a GlobalOpenTelemetry#getIfSet()
method, whose response is @Nullable
and which is null if set has not been called. This would allow callers to atomically check if set has been called and if so, use the result for whatever they need. We could take a naive approach like adding GlobalOpenTelemetry#isSet
, but this has race conditions in it.
The key question here is when would we recommend calling this GlobalOpenTelemetry#getIfSet()
method?
The whole point of GlobalOpenTelemetry
's current design is that it avoids hard-to-debug initialization questions by ensuring that all calls to GlobalOpenTelemetry.get()
get the same result every time. Does adding getIfSet
break that? What about the use case @quaff brings up which is essentially: if the agent is present, I want to use the OpenTelemetry
instance provided by it, else I want to configure my own instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think comparison to
OpenTelemetry.noop()
is the right way to do that.
Could you explain why? the instance returned by public API OpenTelemetry.noop()
is immutable, it is safe enough.
we should add something like a
GlobalOpenTelemetry#getIfSet()
method.
I don't like it, we introduce Noop
to avoid return null
, but it breaks the contract here.
We could take a naive approach like adding
GlobalOpenTelemetry#isSet
I don't like it neither, but it's better than GlobalOpenTelemetry#getIfSet()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why? the instance returned by public API OpenTelemetry.noop() is immutable, it is safe enough.
I think I don't like the idea of having to provide guidance to users of the form: check if GlobalOpenTelemetry.get() == OpenTelemetry.noop()
. It feels weird to have to do that comparison. Also, it gives special meaning / speecial treatment to OpenTelemetry.noop()
when I think the more precise question we're trying to answer is "was GlobalOpenTelemetry set"?
On the other hand, the getIfSet()
and isSet
are ugly because now our guidance becomes more situational:
- Call GlobalOpenTelemetry.get() if you're ok with the side affect of the call initializing GlobalOpenTelemetry
- Call
getIfSet()
/isSet()
if you don't want the side affect
Well, what's our guidance for when you do or do not want the side affect?
I guess I'm softening my stance, but I still wish we could have a more reliable / idiomatic way to signal to the caller that GlobalOpenTelemetry
was not set. On the point of a more reliable signal- its totally possible for someone else to provide their own OpenTelemetry
implementation that is a noop. We essentially do this here - we configure the OpenTelemetrySdk
in such a way that it operates like OpenTelemetry.noop()
, but fulfills the full OpenTelemetrySdk
contract. And yet this instance of OpenTelemetry
is obfuscated and callers of GlobalOpenTelemetry.get()
have no way of knowing that its effectively a noop.
We could do something like add a method: boolean OpenTelemetry#isNoop()
so that implementations could signal that they are a noop in an unambiguous way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do something like add a method:
boolean OpenTelemetry#isNoop()
so that implementations could signal that they are a noop in an unambiguous way.
People may set OpenTelemetry.noop()
or their own noop implementation, still cannot distinguish whether GlobalTelemetry.set()
is called or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well they can't do that with the solution in this PR either.
We'll need to decide whether for use cases like yours, whether: its important to determine that GlobalOpenTelemetry was set, OR that the resolved GlobalOpenTelemetry instance is a noop (remember that GlobalOpenTelemetry.get()
is guaranteed to return the same instance for the entire application lifecycle).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whether: its important to determine that GlobalOpenTelemetry was set, OR that the resolved GlobalOpenTelemetry instance is a noop
Why not both?
I'm using |
Fix inconsistent behavior that first call will return Noop but later calls return obfuscated Noop.